-
Notifications
You must be signed in to change notification settings - Fork 15
feat!: send user agent header #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat!: send user agent header #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Couple comments.
| "Version part should be a semantic version number (e.g., 3.8.4), got: \(versionPart)") | ||
|
|
||
| // Should be the expected SDK version | ||
| XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to update this assertion on every version bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(additional context here) I think this can be improved with release please, yes.
You'll need to add the file to the list of additional files here, and then add comments in here to tell release please where to update.
| XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)") | |
| // x-release-please-start-version | |
| XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)") | |
| // x-release-please-end |
| /// Get the SDK version from the bundle at runtime | ||
| /// Falls back to hardcoded constant or "unknown" if version is not discoverable | ||
| private static func getSDKVersion() -> String { | ||
| // Try CocoaPods bundle first | ||
| if let bundle = Bundle(identifier: "org.cocoapods.FlagsmithClient"), | ||
| let version = bundle.infoDictionary?["CFBundleShortVersionString"] as? String, | ||
| !version.isEmpty, | ||
| version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil { | ||
| return version | ||
| } | ||
|
|
||
| // Try SPM bundle | ||
| if let version = Bundle(for: Flagsmith.self).infoDictionary?["CFBundleShortVersionString"] as? String, | ||
| !version.isEmpty, | ||
| version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil { | ||
| return version | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're OK manually checking a semver-like pattern here, though we could maybe rely on semver parsing for future-proofing, in case we occasionally adopt build or pre-releases metadata.
| /// SDK version constant - should match the podspec version | ||
| /// This is used as a fallback when bundle version detection fails | ||
| private static let sdkVersionConstant = "3.8.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the resilience this adds given the platform, though this may prompt for some process that ensures code will be kept in sync with the actual version tagged.
@polatolu Do you think we need to implement some kind of CI script to accomplish this? Alternatively, would you consider falling back to "unknown" when bundle version detection fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible we need to add this to the release please config as well to resolve this? See instructions here.
… latest Swift as wasn't picking up the test-config.json
Description
Added a User-Agent HTTP header to network calls. Uses the package's version at runtime, if it is not available sends it as "unknown"
Also added a PR template, once this merged new PRs will come with this PR template.
Regression Test Recommendations
Type of Change
🛠️ Bug fix (non-breaking change which fixes an issue)❌ Breaking change (fix or feature that would cause existing functionality to change)🧹 Code refactor✅ Build configuration change📝 DocumentationUnit Test Results